Separate submitted and response salts in UI filters - #167
Conversation
Validate the salt submitted with the request rather than a request attribute, and run validation before the response salt is generated. Move multipart validation into a Struts interceptor after the upload interceptor, since filters cannot read multipart fields. Drop the unused salt.ignored.urls bypass. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
There was a problem hiding this comment.
Two regressions need fixing before merge, because making salts genuinely single-use exposes flows that only worked thanks to the bypass:
- A multipart action with a
chainresult (bookmarksImport!save→bookmarks) is validated twice; the second pass finds the salt already consumed and throws after the import has committed. (inline) Comments.jspreuses one#comments_saltfor every inline AJAX save, so the second save now fails silently. (inline)
|
|
||
| // Remove salt from cache after successful validation | ||
| saltCache.remove(salt); | ||
| if (!SaltValidator.consumeSubmittedSalt(httpReq)) { |
There was a problem hiding this comment.
Comments.jsp:429 reads #comments_salt once and sends it on every commentdata AJAX POST. With the salt removed on first use, editing a second comment without reloading fails here with a 500, and the $.ajax call has no error handler, so the save just silently doesn't happen. CommentDataServlet could return a fresh salt in its JSON (the response passes through LoadSaltFilter, so request.getAttribute("salt") is available) and the JS update #comments_salt from it.
|
|
||
| SaltCache saltCache = SaltCache.getInstance(); | ||
| synchronized (saltCache) { | ||
| if (!Objects.equals(saltCache.get(salt), userId)) { |
There was a problem hiding this comment.
Now that this is the only path, cache.salt.size (5000) and cache.salt.timeout (3600s) are user-visible limits: LoadSaltFilter mints a salt on every /roller-ui request and on every tiles FORWARD, so entries get evicted quickly on a multi-user site, and a form left open for over an hour is rejected with the entry text lost. Not a blocker, but please call it out in the description; raising the defaults would soften it.
|
|
||
| @Test | ||
| public void testSubmittedSaltIsValidatedBeforeResponseSaltIsLoaded() throws Exception { | ||
| String webXml = Files.readString(Path.of("src/main/webapp/WEB-INF/web.xml")); |
There was a problem hiding this comment.
cwd-relative, so this passes only when run from app/; the other two tests in this class use getResourceAsStream, and surefire sets project.build.directory for this module.
| <!-- <interceptor-ref name="scopedModelDriven"/> --> | ||
| <!-- <interceptor-ref name="modelDriven"/> --> | ||
| <interceptor-ref name="fileUpload"/> | ||
| <interceptor-ref name="ValidateSaltInterceptor"/> |
There was a problem hiding this comment.
The comment in ValidateSaltFilter says multipart params only exist after fileUpload, but Struts wraps the request in MultiPartRequestWrapper in StrutsPrepareAndExecuteFilter before any interceptor runs; fileUpload only copies file items into action params. So this ref could sit right after exception (validate before anything else does work), and SaltConfigurationTest:51 shouldn't pin it as adjacent to fileUpload.
| log.debug("Valid salt value not found on POST to URL : " | ||
| + httpReq.getServletPath()); | ||
| } | ||
| throw new ServletException("Security Violation"); |
There was a problem hiding this comment.
Nit: this block is duplicated in the interceptor; a SaltValidator.requireSubmittedSalt(request) that throws would keep both rejection paths in sync.
| } | ||
|
|
||
| @Test | ||
| public void testValidationRunsBeforeResponseSaltGeneration() throws Exception { |
There was a problem hiding this comment.
This builds the chain in validate-then-load order and then asserts that order, so it can't fail if web.xml is swapped back; SaltConfigurationTest already covers the ordering.
| String properties = readResource( | ||
| "/org/apache/roller/weblogger/config/roller.properties"); | ||
|
|
||
| assertFalse(properties.contains("salt.ignored.urls")); |
There was a problem hiding this comment.
Asserting the literal salt.ignored.urls is absent from roller.properties means a migration note like # salt.ignored.urls is no longer supported breaks the build. The behaviour is already covered by the filter no longer reading the property.
2 participants
Roller's UI CSRF protection uses two salt filters: one checks the token
submitted with a request, the other generates the token for the next response.
This change gives each filter a single responsibility and corrects the order in
which they run.
What changed
saltvalue submitted with the request.only after a successful validation, and terminate rejected POSTs without a
replacement token.
*.rolvalidation to an interceptor that runs immediatelyafter the upload interceptor, and reject non-Struts multipart requests that
lack a submitted token.
salt.ignored.urlssetting and itsisIgnoredURL()helper, whichdid not match the shipped values correctly.
Tests
saltis rejected; one valid tokensucceeds once and a second use of the same token fails.
already-used tokens, exercised on the supported Tomcat deployment and the
repository Jetty test setup.